fix(tests): tag every tier a test belongs to, not just the invoked one - #847
fix(tests): tag every tier a test belongs to, not just the invoked one#847ashokponkumar wants to merge 1 commit into
Conversation
result_tags() stamped one testtype__ tag, from SPYRE_TEST_TIER -- the tier the run was
INVOKED as. But a test's tier membership is a set: _test_matrix.yaml declares it per
matrix entry as `test_types`, and 11 of the 32 entries declare four tiers
("unit integration regression trunk").
So a regression run recorded only testtype__regression on tests that are equally unit
and integration members. Anything asking "has this artifact already been tested at tier
X" then finds nothing and re-executes identical work -- the tags existed but could never
satisfy a coverage query.
The membership set was already declared and already correct; it was simply never handed
to pytest. matrix.test_types is in scope at both run-matrix-config call sites (which
already pass matrix.cfg and matrix.runs_on) and was the one field not passed down. It now
threads through as SPYRE_TEST_TIERS and yields one tag per member.
Membership is READ, never inferred from a tier ladder. 13 of the 32 entries declare
`unit regression trunk` without `integration`, so closing the ladder upward would claim
integration coverage for 13 legs that never ran it and silently skip real tests. A test
asserts a ladder would still over-claim this matrix, so the shortcut cannot creep back in.
The invocation tier is deliberately NOT tagged. Case tags are hashed into test_case_id
(ingest_xml_si.v2_test_case_id), so tagging it gave one test a different identity per
invoking tier -- verified: the same test hashed to three uuids. It is a property of the
run, and si_test_runs.test_type already records it.
Verified end to end: a probe suite emits three testtype__ tags where it previously
emitted one, `integration` correctly absent; the ingest already on main parses all three
with no change, and the identity is now stable across invoking tiers. The pod-level retry
needs nothing -- the failed-suite descriptor is toJSON(matrix), so test_types is rebuilt
with the rest of the entry. Two mutations (reverting to one tier, and closing the ladder)
each fail the new tests.
Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
|
👋 Hi! Thank you for contributing. We also recommend installing prek and configuring it to check your code before every local commit. |
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Raw test tier; resolved via `make print-test-type` and exported as | ||
| SPYRE_TEST_TIER for the testtype__<tier> tag. Empty = Makefile default. | ||
| Raw test tier this run was INVOKED as; resolved via `make print-test-type` and | ||
| exported as SPYRE_TEST_TIER for the invoked_as__<tier> tag. Empty = Makefile |
There was a problem hiding this comment.
This doc now points at an invoked_as__<tier> tag that is never emitted. The PR deliberately stops tagging the invoked tier as a case tag (see test_tier() docstring and test_the_invocation_is_never_a_case_tag), so no invoked_as__ tag exists anywhere. Suggest describing what SPYRE_TEST_TIER is actually used for now: the run-row test_type and the test_tiers() fallback.
| doc = yaml.safe_load(_MATRIX.read_text(encoding="utf-8")) | ||
| jobs = doc["jobs"] | ||
| include = jobs["test"]["strategy"]["matrix"]["include"] | ||
| return [entry["test_types"].split() for entry in include if entry.get("test_types")] |
There was a problem hiding this comment.
The actual behavior this PR changes -- the wiring -- is not covered by any test. Nothing asserts that run-matrix-config exports SPYRE_TEST_TIERS from its new test_types input, and nothing asserts that _test_matrix.yaml passes test_types: ${{ matrix.test_types }} at either call site. Delete either wire and every test here still passes; the tags silently degrade to the invoked-tier fallback in CI. The PR description says "both call sites asserted wired," but I do not see that assertion -- these tests only exercise tags.py in-process plus the matrix declarations. Consider a small YAML-parse test that greps both call sites for test_types: ${{ matrix.test_types }} and the action for the SPYRE_TEST_TIERS export.
| """Guards the source of truth itself: if `test_types` ever disappears from the | ||
| matrix, the tags silently degrade to the invoked tier via the fallback.""" | ||
| sets = _declared_sets() | ||
| assert len(sets) >= 20, f"expected the full shard matrix, got {len(sets)} entries" |
There was a problem hiding this comment.
This guard is looser than its docstring claims. The matrix has 32 entries; >= 20 lets up to 12 entries drop test_types (and silently fall back to the invoked tier) without failing. Since _declared_sets() filters out entries with no test_types, a per-entry regression is invisible here. Assert that every include entry declares test_types (e.g. compare against the full include count) rather than a floor of 20.
|
Reviewed the tier-tagging change: the No Spyre hardware was needed for this producer-only change. I ran the 13 CPU tests locally (all pass) and confirmed via Three inline comments, all low-to-medium: the actual wiring (action export plus both call sites passing |
2 participants
Problem
result_tags()stamps onetesttype__tag, taken fromSPYRE_TEST_TIER— the tier the run was invoked as. But tier membership is a set:_test_matrix.yamldeclares it per matrix entry astest_types, and 11 of the 32 entries declare four tiers (unit integration regression trunk).So a
regressionrun records onlytesttype__regressionon tests that are equallyunitandintegrationmembers. Anything asking "has this artifact already been tested at tier X?" finds nothing and re-executes identical work. The tags exist today but can never satisfy a coverage query — which is what the v2 tier-delta work needs them for.Fix
The membership set was already declared and already correct — it was simply never handed to pytest.
matrix.test_typesis in scope at bothrun-matrix-configcall sites (which already passmatrix.cfg,matrix.test_target,matrix.runs_on); it was the one field not passed down.run-matrix-config: newtest_typesinput, exported asSPYRE_TEST_TIERS_test_matrix.yaml: passmatrix.test_typesat both call sites (main + retry)tags.py:test_tiers()reads the set; onetesttype__<tier>tag per member. Falls back to the invoked tier when unset, so a localmake testand any un-updated caller keep tagging.Two things worth reviewer attention
Membership is read, never inferred from a ladder. 13 of the 32 entries declare
unit regression trunkwithoutintegration. Closing the ladder upward ("in unit ⇒ in everything above") would claim integration coverage for 13 legs that never ran it and silently skip real tests. There's a test asserting a ladder would still over-claim this matrix, so the shortcut can't creep back in.The invocation tier is deliberately NOT tagged. Case tags are hashed into
test_case_id(ingest_xml_si.v2_test_case_id), so tagging it gave one test a different identity per invoking tier — I verified the same test hashing to three different uuids before removing it. The invocation is a property of the run, andsi_test_runs.test_typealready records it.Verification
testtype__tags where it previously emitted one, withintegrationcorrectly absentsi_run_propertiesrow per tag) — no ingest change neededtest_case_idis stable across invoking tiers (was three uuids, now one)toJSON(matrix), sotest_typesis rebuilt with the rest of the entryScope
Producer only. No change to which tests run — only to what the JUnit XML records. Safe to land before the consumer, and worth landing first: tags feed the v2 identity hash, and
spyre_v2.test_case_runscurrently holds 0 rows forspyre-inference, so re-keying costs nothing now and would orphan identities later.